ci: run the orphaned test/db regression runners (and make them portable) - #153
Merged
Conversation
test/db/ holds four runners that each guard a specific fixed bug, but NONE was invoked by any CI job or build target, so a regression in the code they cover would have gone unnoticed: run_hash_unsorted_cmp the #139 hash comparator fix (PR #144) run_recd_compact compact-path recovery run_recd_handlers recovery handler dispatch run_upgrade the on-disk upgrade path Found while verifying that the #139 fix on master really contains BOTH required changes (it does: LEN_HKEY for the stored item's length, and capturing the comparator's result). The test proving that is thorough -- it covers the false-negative, the false-positive-on-prefix that appears if only the reported half is fixed, and an over-long key -- but nothing ran it. Added to the 'default' matrix config of the existing build job, which already builds build_unix with the shared library. All four follow the same BUILD=${BUILD:-.} convention and run from the build directory. Verified locally with the exact CI command sequence: 4/4 PASS. actionlint reports no findings for the new step.
Coccinelle convention checksNo new violations. ✅ Resolved since baseline (2) -- update dist/cocci/baseline.txt to lock these in. |
ABI diff vs
|
Wiring these runners into CI (previous commit) exposed a real portability gap: the library probe looked only for .libs/libdb-*.so, so all four macOS jobs failed with 'libdb .so not found'. macOS builds libdb-5.3.dylib. The probe now tries the exact .so and .dylib names first, then the globs for each, and its failure message names both suffixes. The -Wl,-rpath linking already works on both platforms, so nothing else needed changing. Verified: 4/4 PASS on Linux (unchanged), and the new probe resolves a .dylib-only build directory.
Second macOS gap exposed by wiring these runners into CI. The .dylib probe fixed the library lookup, and the runners then compiled and linked fine on macOS, but all four still failed with rc=127: 'timeout: command not found'. GNU coreutils' timeout is not on stock macOS, where it is gtimeout if coreutils is installed. Added a run_with_timeout helper to each runner: prefer timeout, then gtimeout, and if neither exists run the command WITHOUT a timeout rather than failing -- losing the timeout is much better than reporting a false failure. Both branches were exercised explicitly, including that the no-timeout path drops the leading seconds argument. 4/4 still PASS on Linux.
Third and root-cause macOS gap. After the .dylib probe and the timeout helper,
the binaries compiled and linked on macOS but aborted at startup:
dyld: Library not loaded: /usr/local/BerkeleyDB.5.3/lib/libdb-5.3.dylib
The .dylib carries a baked-in install name, and on macOS the install name takes
precedence over -Wl,-rpath, so the binary looked in an uninstalled location. The
runners never actually needed dynamic linking -- the other CI-wired suites
(test/fuzz) link libdb.a -- so they now PREFER the static library and only fall
back to shared (adding -rpath in that case). This removes the dynamic loader from
the picture instead of fighting it.
Also replaced the hardcoded 'gcc' with "${CC:-cc}": the macOS jobs run clang,
and hardcoding gcc would have been the next failure in this same sequence.
Verified: 4/4 PASS locally and 'ldd hash_unsorted_cmp | grep -c libdb' = 0,
confirming the static link.
gburd
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
test/db/holds four runners that each guard a specific fixed bug, but none was invoked by any CI job or build target — so a regression in the code they cover would have gone unnoticed.run_hash_unsorted_cmprun_recd_compactrun_recd_handlersrun_upgradeHow this surfaced
While double-checking that the #139 fix on master really contains both required changes. It does —
LEN_HKEY(dbp, p, dbp->pgsize, i)for the stored item's length andres = t->h_compare(...)capturing the result. The second matters: with only the reported half, a prefix search key compares equal against a truncated view of the stored key, turning the false negative into a false positive (get("acct")returningacct0000's data). The test proving this is thorough — false-negative, false-positive-on-prefix, over-long key, andDB_NOOVERWRITE— but nothing ran it.Wiring them up exposed three real portability bugs
Enabling them on the existing build matrix made all four macOS jobs fail, each time for a different reason. All three were genuine latent defects in the runners, not CI noise:
.libs/libdb-*.so; macOS buildslibdb-5.3.dylib.timeoutis GNU coreutils — absent on stock macOS (rc=127). Added arun_with_timeouthelper:timeout→gtimeout→ run without one. Losing the timeout beats a false failure. Both branches exercised explicitly, including that the fallback drops the seconds argument..dylib's baked-in install name (/usr/local/BerkeleyDB.5.3/lib/...) takes precedence over-Wl,-rpathon macOS, so the binary aborted indyldagainst an uninstalled path. The runners never needed dynamic linking, so they now preferlibdb.a(whattest/fuzzalready does) and only fall back to shared. Also replaced hardcodedgccwith"${CC:-cc}", which would have been the next failure.Verification
ldd hash_unsorted_cmp | grep -c libdb= 0, confirming the static link.Compiling hash_unsorted_cmp against ./libdb.athen all four.sh: PASS.actionlintclean for the new step.No engine changes.